-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
add version
build form sources to get rid of the downgrades
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for doing this upgrade. Some minor comments, and quite some questions about the reasoning of some of these changes, so it’s easier in the future to know if something is still needed or not.
A summary in the merge/pull request would also be nice. I guess the biggest change is to build Tensorflow and hic2cool from source?
@@ -1,14 +1,14 @@ | |||
#! /bin/bash | |||
|
|||
COOKIE=$(mcookie); grep -v V_GREP_ME $0 > /dev/shm/runme-$COOKIE.sh ; sleep 1; exec bash /dev/shm/runme-$COOKIE.sh $1 | |||
#COOKIE=$(mcookie); grep -v V_GREP_ME $0 > /dev/shm/runme-$COOKIE.sh ; sleep 1; exec bash /dev/shm/runme-$COOKIE.sh $1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is that not needed anymore? It’d be great if you mentioned that in the commit message (and also prefix it with python-3.7.6:
). Maybe even delete that line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is a dev line
copy the script to /dev/shm and run there
so you can edit while building
@@ -175,7 +175,7 @@ make install | |||
python3 -m ensurepip | |||
pip3 install --prefix=$PREFIX -I pip | |||
|
|||
pip3 install --prefix=$PREFIX ipython[all] | |||
pip3 install --upgrade --prefix=$PREFIX ipython[all] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is --ugprade
added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I have add this for rebuilding to find the downgrader
|
||
# pip3 install --prefix=$PREFIX HiCExplorer # Would be a nice to have, but it likes to downgrade too much, pick some cherries | ||
pip3 install --prefix=$PREFIX HiCMatrix | ||
pip3 install --prefix=$PREFIX pyGenomeTracks | ||
pip3 install --prefix=$PREFIX pyGenomeTracks #downgrader cooler 0.8.7-> 0.8.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn’t that also the version pinned in the hic2cooler requirements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by hic2cool is the problem numpy and scipy and not cooler
--cudart-lib-dir='${CUDA_ROOT}/lib64,${CUDA_ROOT}/lib64/stubs' \ | ||
--curand-lib-dir='${CUDA_ROOT}/lib64,${CUDA_ROOT}/lib64/stubs'\ | ||
--ldflags=-L$PREFIX/lib | ||
--cuda-enable-gl \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use tabs to indent this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
next time I promise :-)
python3 setup.py install --prefix $PREFIX | ||
) | ||
|
||
# hic2cool from source to stop downgrading python packages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which requirements are a problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
numpy and scipy
|
||
#tensorflow | ||
( | ||
TFVERSION=2.1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to install Tensorflow from source? Why is the package not good enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f.e. the half of the cluster dont work with the pip package
./configure | ||
bazel build \ | ||
--cxxopt="-march=nehalem" \ | ||
--copt="-march=nehalem" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do all our servers support this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, but the important do it. This activate SSE4.1
this server f.e. dont support it
botanophobie
bromhidrosophobie
bugblatterbeastoftraal
carnophobie
deathofrats
deathofrats
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you want I can send you the full list
python-3.7.6-0.build.sh
Outdated
//tensorflow:install_headers \ | ||
//tensorflow/tools/pip_package:build_pip_package | ||
|
||
test -d tensorflow-pip && rm -rf tensorflow-pip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check is not needed, as rm -rf
will exit with success even if the directory does not exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changed it d0c427b
python-3.7.6-0.build.sh
Outdated
#remove symlinks into TMPDIR | ||
test -e tensorflow-pip.tar && rm tensorflow-pip.tar | ||
tar -chf tensorflow-pip.tar tensorflow-pip | ||
test -d tensorflow-pip && rm -rf tensorflow-pip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto. The check is not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
Inline comments don't work either ... The check is not needed, as rm -rf will exit with success even if the directory does not exist. If a directory doesn't exist, there is no need to try a removal, or? |
Build on theinternet needed 6 hours :-) I've needed to remove
because pyGenomeTracks was updated on PyPi and not requires a version of HicMatrix which is not yet available on PyPi or some such nonsense. |
The stuff from the HiC-clan is a real pita. |
When this was build into /package/pkg everything was fine. Until we change the system very basically, we'll always have the problem, that it is not reproducible because of PyPi updates. Thats not a fault of the this package. No change justified, imo. updates unless we change something |
On which cuda machines should this tensorflow work? cpu was okay, but on "aros" I get |
aros is using the former nvidia-driver (Driver Version: 418.56 CUDA Version: 10.1). You might try dalek (Driver Version: 440.44 CUDA Version: 10.2). Meaning the GPU version will only run if you are using a kernel that uses the 440.44 version (or above) of the NVIDIA driver. |
Works fine on dalek :-) |
Dalek is the new GPU - beast. And in case you like Dr. Who, just scream 'eliminaaaaaaate' :) |
hic2cool version 0.8.2 loosened the dependency requirements. I urge everyone to engage with upstream. |
EOFHIC | ||
fi | ||
pip3 install -r requirements.txt --prefix ${PREFIX} | ||
python3 setup.py install --prefix ${PREFIX} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/4dn-dcic/hic2cool/pull/44#issuecomment-605348278 claims, pip3 install
is the recommended way to build and install packages.
now a clean PR